[https://nvbugs/6482576][fix] Fall back to disagg_request_id in Python NIXL decode receiver#16720
Conversation
f297be0 to
47a2999
Compare
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughChangesDisaggregated request ID handling
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
/bot run --disable-fail-fast |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tensorrt_llm/_torch/disaggregation/native/transfer.py`:
- Around line 1498-1506: Replace the assert guarding sender_req_id in the task
request-ID fallback flow with an explicit ValueError when both
task._params.ctx_request_id and task._params.disagg_request_id are missing, so
validation remains active under optimization. Add a regression test covering the
both-IDs-missing case and verify that ValueError is raised.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 2064cd02-f1c2-4b25-9a0c-651990db4cb8
📒 Files selected for processing (2)
tensorrt_llm/_torch/disaggregation/native/transfer.pytests/unittest/disaggregated/test_request_id.py
|
PR_Github #60942 [ run ] triggered by Bot. Commit: |
47a2999 to
b8cb01e
Compare
|
/bot run --disable-fail-fast |
b8cb01e to
23958d5
Compare
|
PR_Github #60947 [ run ] triggered by Bot. Commit: |
|
/bot run --disable-fail-fast |
|
PR_Github #60942 [ run ] completed with state |
|
PR_Github #60954 [ run ] triggered by Bot. Commit: |
|
PR_Github #60947 [ run ] completed with state |
|
PR_Github #60954 [ run ] completed with state
|
23958d5 to
b2f8d78
Compare
|
/bot run --disable-fail-fast |
|
PR_Github #60971 [ run ] triggered by Bot. Commit: |
|
PR_Github #60971 [ run ] completed with state
|
…n NIXL decode receiver In disaggregated serving with cache_transceiver_config.backend=NIXL and transceiver_runtime=PYTHON, a generation request can reach the decode worker with disagg_request_id populated but ctx_request_id None. Receiver._build_recv_req_info() asserted ctx_request_id is not None, which crashed that decode rank's event loop and left peer ranks hanging in MPI collectives. sender_req_id keys the ctx TxSession but is never read back by the sender's _respond_with_kv (which keys on unique_rid), so fall back to disagg_request_id when ctx_request_id is None. Raise ValueError (not assert, so it survives python -O) if both ids are None, so a None id never reaches RecvReqInfo. Add unit tests in test_request_id.py covering the ctx-preferred, disagg fallback, and both-None cases. Signed-off-by: Xiaowei Shi <39303645+Shixiaowei02@users.noreply.github.com>
b2f8d78 to
346f26d
Compare
|
/bot run --disable-fail-fast |
|
PR_Github #61155 [ run ] triggered by Bot. Commit: |
nv-xtf
left a comment
There was a problem hiding this comment.
Just curious — is ctx_request_id being gradually phased out in favor of disagg_request_id?
|
PR_Github #61155 [ run ] completed with state
|
Yes, we'd like to keep only a single ID, as that's the cleanest design. The |
|
/bot run --disable-fail-fast |
|
PR_Github #61262 [ run ] triggered by Bot. Commit: |
|
PR_Github #61262 [ run ] completed with state
|
|
/bot run --disable-fail-fast |
|
PR_Github #61452 [ run ] triggered by Bot. Commit: |
|
PR_Github #61452 [ run ] completed with state
|
|
/bot skip --comment "Related tests have passed." |
|
PR_Github #61534 [ skip ] triggered by Bot. Commit: |
|
PR_Github #61534 [ skip ] completed with state |
…n NIXL decode receiver (NVIDIA#16720) Signed-off-by: Xiaowei Shi <39303645+Shixiaowei02@users.noreply.github.com>
Description
This pull request improves the handling of request IDs in the disaggregated receiver logic and adds new tests to ensure correct fallback behavior when
ctx_request_idis missing. The changes address a known bug (nvbugs/6482576) where the receiver could crash ifctx_request_idisNone, by falling back todisagg_request_idinstead. Additionally, tests are updated and expanded to cover these scenarios.Bug fix and fallback logic:
_build_recv_req_infointransfer.pyto fall back todisagg_request_idwhenctx_request_idisNone, preventing receiver crashes on certain requests. An assertion ensures at least one ID is present.Test improvements:
test_request_id.pyto verify the receiver's fallback logic and assertion for missing IDs. [1] [2]These changes make the receiver more robust and ensure test coverage for the new behavior.
Test Coverage
PR Checklist
Please review the following before submitting your PR:
PR description clearly explains what and why. If using CodeRabbit's summary, please make sure it makes sense.
PR Follows TRT-LLM CODING GUIDELINES to the best of your knowledge.
Test cases are provided for new code paths (see test instructions)
If PR introduces API changes, an appropriate PR label is added - either
api-compatibleorapi-breaking. Forapi-breaking, includeBREAKINGin the PR title.Any new dependencies have been scanned for license and vulnerabilities
CODEOWNERS updated if ownership changes
Documentation updated as needed
Update tava architecture diagram if there is a significant design change in PR.
The reviewers assigned automatically/manually are appropriate for the PR.
Please check this after reviewing the above items as appropriate for this PR.
GitHub Bot Help
To see a list of available CI bot commands, please comment
/bot help.Dev Engineer Review
Receiver._build_recv_req_infosender request-id handling for disaggregated (NIXL decode) receiver flows by preferringtask._params.ctx_request_idand falling back totask._params.disagg_request_idwhenctx_request_idisNone(nvbugs/6482576).ctx_request_idnon-Noneassertion with explicit error handling: if bothctx_request_idanddisagg_request_idareNone, the code now raisesValueErrorto preventNonefrom reachingRecvReqInfo.sender_req_id/the wire (still keepstask._unique_ridnon-Noneassertion).QA Engineer Review
tests/unittest/disaggregated/test_request_id.py:test_build_recv_req_info_prefers_ctx_request_idtest_build_recv_req_info_falls_back_to_disagg_request_idtest_build_recv_req_info_both_ids_none_raisesReceiver._build_recv_req_infoas an unbound method with a mocked registrar.tests/integration/test_lists/,test-db/,qa/,waives.txt) were not updated since this is unit-test coverage only.